Skip to content

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Jun 4, 2021

Follow-up to #85788
cc @rylev

@rust-highfive
Copy link
Contributor

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 4, 2021
@ehuss ehuss mentioned this pull request Jun 7, 2021
4 tasks
Copy link
Member

@rylev rylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that this uses the existing infrastructure, but I personally find it a tiny bit more complicated to reason about since --force-warns isn't really a logical level. But I'm happy either way. 😊

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay in reviewing, I feel like this is an improvement to me, but I appreciate that it isn't logically a lint level. r=me if you and @rylev are happy with it.

@cjgillot
Copy link
Contributor Author

I really have trouble understanding why you consider it as not being a lint level. The way I see it, force-warns is to warn what forbid is to error. Am I missing something?

@rylev
Copy link
Member

rylev commented Jun 17, 2021

Hmmm error is not a lint level. Did you mean deny? If so, then I see where you're coming from.

My issue is that Level is also used for lint definitions, and it didn't make sense to me for a lint to define its level as being ForceWarn. However, I now see that it might make sense for there to be a level that is logically like Warn but that cannot be turned off. Just like Forbid is like Deny but cannot be turned off.

I believe my issue is one of nomenclature. From the command line, the user is logically forcing a certain lint to warn so the name "force-warn" makes sense. But at the lint definition site, if you sent level to ForceWarn you're really saying, we want that this always emits a warning so perhaps the name AlwaysWarn makes more sense.

I'm largely in favor this change now, but two questions:

  • Does the name ForceWarn actually make sense? Since the command line option is unstable, it's easy for us to change the name.
  • Can we document somewhere the analogy of force-warn is to warn as forbid is to deny? This should clarify things.

@bors
Copy link
Collaborator

bors commented Jun 25, 2021

☔ The latest upstream changes (presumably #86627) made this pull request unmergeable. Please resolve the merge conflicts.

@davidtwco davidtwco added S-waiting-on-bikeshed Status: Awaiting a decision on trivial things. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 28, 2021
@davidtwco
Copy link
Member

r? @rylev

@rust-highfive rust-highfive assigned rylev and unassigned davidtwco Jun 28, 2021
@cjgillot
Copy link
Contributor Author

Hmmm error is not a lint level. Did you mean deny? If so, then I see where you're coming from.

Yes

My issue is that Level is also used for lint definitions, and it didn't make sense to me for a lint to define its level as being ForceWarn. However, I now see that it might make sense for there to be a level that is logically like Warn but that cannot be turned off. Just like Forbid is like Deny but cannot be turned off.

I believe my issue is one of nomenclature. From the command line, the user is logically forcing a certain lint to warn so the name "force-warn" makes sense. But at the lint definition site, if you sent level to ForceWarn you're really saying, we want that this always emits a warning so perhaps the name AlwaysWarn makes more sense.

I chose ForceWarn to mirror the command line option. Happy to change.

I'm largely in favor this change now, but two questions:

* Does the name `ForceWarn` actually make sense? Since the command line option is unstable, it's easy for us to change the name.

* Can we document somewhere the analogy of force-warn is to warn as forbid is to deny? This should clarify things.

I will do both changes at once if you decide to change the name.

@rylev
Copy link
Member

rylev commented Jun 29, 2021

@cjgillot Actually, let's merge this first. We can bike shed the name in a new PR as part of stabilization.

@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 29, 2021

📌 Commit e42271d has been approved by davidtwco

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 29, 2021
@bors
Copy link
Collaborator

bors commented Jun 29, 2021

⌛ Testing commit e42271d with merge 8971fff...

@bors
Copy link
Collaborator

bors commented Jun 29, 2021

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 8971fff to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 29, 2021
@bors bors merged commit 8971fff into rust-lang:master Jun 29, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 29, 2021
@ehuss
Copy link
Contributor

ehuss commented Jun 30, 2021

This seems to have broken interaction with --force-warns and --cap-lints. Cargo does the following when running cargo fix --edition:

rustc main.rs --force-warns=rust-2021-compatibility -Zunstable-options --cap-lints=warn

The lints no longer fire on an example:

#![allow(ellipsis_inclusive_range_patterns)]

pub fn f() -> bool {
    let x = 123;
    match x {
        0...100 => true,
        _ => false,
    }
}

This is somewhat different from #86572, since --cap-lints=warn shouldn't set can_emit_warnings.

Can you take a look at this?

@rylev
Copy link
Member

rylev commented Jul 8, 2021

The interaction with cap-lints has been fixed in #86572.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bikeshed Status: Awaiting a decision on trivial things. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants